-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: change the color palette #48
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/storybook/design/Shadows.mdx
Outdated
@@ -28,7 +28,7 @@ export const Shadows = () => { | |||
Display | |||
</Typography> | |||
</Grid> | |||
{Object.keys(theme.shadows).map((index) => ( | |||
{Object.keys(theme.shadows.slice(0, 5)).map((index) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did this because for the design system we only have 4 variants of shadow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcosh-ramos, this will cause a breaking change though. The MUI theme type accepts 25 variants, so we must adhere to it.
At a later point, we could create a custom theme, but for now we must adhere to the type.
Perhaps we can make every 5 shadows the same?
- 0-4 -> 0
- 5-9 -> 1
- etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be strange have the shadows from 0 to 4 mapping to none, at least was not was I would expect if I use shadows[1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back to what was before. for me would be worst to map like this because people that now use the shadows from 1 to 4 would lose it with this update.
changes:
e66a71c#diff-eaaeb6908dd041479365a278ba68513d6ffb3512501d44999c6e1685cfa89cb1L31
e66a71c#diff-f2727474dc5684e74bd35e134abe55e2a53454471a4f806c72942be9b1b0b458R56-R67
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcosh-ramos Nice! I found some breaking changes though.
We'd need a more detailed better description, so that it's clear what we are updating. We will need to communicate all of this in detail in the release notes anyways.
src/storybook/design/Shadows.mdx
Outdated
@@ -28,7 +28,7 @@ export const Shadows = () => { | |||
Display | |||
</Typography> | |||
</Grid> | |||
{Object.keys(theme.shadows).map((index) => ( | |||
{Object.keys(theme.shadows.slice(0, 5)).map((index) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcosh-ramos, this will cause a breaking change though. The MUI theme type accepts 25 variants, so we must adhere to it.
At a later point, we could create a custom theme, but for now we must adhere to the type.
Perhaps we can make every 5 shadows the same?
- 0-4 -> 0
- 5-9 -> 1
- etc
- coming back with the old color variables - coming back with the 5px to the base_border_radius - coming back with the old shadows
@marcosh-ramos I made a couple of adjustments to this PR and merged it. I am going to release it as @storyblok/[email protected] |
closes #35
closes #45
What?
With this task we are changing the colors to use the new variants introduced by storyblok.
Why?
How to test? (optional)
Open the preview and check the palette of colors and how it looks in the components.